Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: get trainer from model_name in weight #1744

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

sunya-ch
Copy link
Collaborator

Related to the comment in #1728 (comment) and sustainable-computing-io/kepler-model-server#414, for now, we can extract the trainer name from the model name.

Signed-off-by: Sunyanan Choochotkaew sunyanan.choochotkaew1@ibm.com

Copy link

github-actions bot commented Aug 28, 2024

🤖 SeineSailor

Here is a concise summary of the pull request changes:

Summary: This pull request introduces changes to the regressor package, config package, and model package to extract the trainer name from the model name. The key modifications include:

  • Adding Trainer() and isSupportedTrainer() functions to the Regressor struct
  • Updating getWeightFromServer and loadWeightFromURLorLocal functions to set the TrainerName field
  • Extracting the trainer name from the model name in the config package
  • Adding a new GetModelNameFromURL function to the utils package
  • Updating tests in the model_test.go file to cover the new functionality

Impact: These changes are internal and do not alter the external interface or behavior of the code. They address issues in pull request #1728 and issue #414, where the trainer name is extracted from the model name.

Observations:

  • The changes are well-contained within the respective packages and do not have a broad impact on the codebase.
  • The addition of the GetModelNameFromURL function in the utils package is a useful utility that can be reused in other parts of the codebase.
  • The tests added in the model_test.go file provide good coverage for the new functionality.

Suggestions for improvement:

  • Consider adding documentation or comments to explain the purpose and usage of the new GetModelNameFromURL function.
  • Review the WeightSupportedTrainers global variable to ensure it is properly synchronized across the codebase.
  • Consider refactoring the getWeightFromServer and loadWeightFromURLorLocal functions to make them more modular and reusable.

@rootfs
Copy link
Contributor

rootfs commented Aug 28, 2024

@sunya-ch can you rebase? thanks

@@ -1 +1 @@
{"platform": {"All_Weights": {"Bias_Weight": 220.9079278650894, "Categorical_Variables": {}, "Numerical_Variables": {"bpf_cpu_time_ms": {"scale": 5911.969193263386, "mean": 0, "variance": 0, "weight": 29.028228361462897}}}}}
{"model_name": "SGDRegressorTrainer_0", "platform": {"All_Weights": {"Bias_Weight": 220.9079278650894, "Categorical_Variables": {}, "Numerical_Variables": {"bpf_cpu_time_ms": {"scale": 5911.969193263386, "mean": 0, "variance": 0, "weight": 29.028228361462897}}}}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have the trainer_name instead?
I am fine with model_name but I think we shouldn't require parsing model_name to get trainer name.

Comment on lines +129 to +130
func (w ComponentModelWeights) Trainer() string {
if w.ModelName == "" {
return ""
}
modelNameSplits := strings.Split(w.ModelName, "_")
splitTrainer := strings.Join(modelNameSplits[0:len(modelNameSplits)-1], "_")
if isSupportedTrainer(splitTrainer) {
return splitTrainer
}
return ""
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, models should have the trainer-name metadata embedded in the json itself so that this isn't needed.

Copy link
Collaborator Author

@sunya-ch sunya-ch Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

model_name has more complete information since it also embed the node_type and linked to model db.
I think we can add trainer_name additionally if needed (need update on model-server side first).

Signed-off-by: Sunyanan Choochotkaew <sunyanan.choochotkaew1@ibm.com>
@sthaha sthaha merged commit 79a34f8 into sustainable-computing-io:main Aug 29, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants